feat: support multiple value for pivot #1970
Conversation
src/ast/query.rs
Outdated
| @@ -1336,7 +1336,7 @@ pub enum TableFactor { | |||
| Pivot { | |||
| table: Box<TableFactor>, | |||
| aggregate_functions: Vec<ExprWithAlias>, // Function expression | |||
| value_column: Vec<Ident>, | |||
| value_column: Vec<Expr>, // Expr is a identifier or a compound identifier | |||
There was a problem hiding this comment.
| value_column: Vec<Expr>, // Expr is a identifier or a compound identifier | |
| value_column: Vec<Expr>, |
tests/sqlparser_common.rs
Outdated
| assert_eq!( | ||
| verified_stmt(sql_with_multiple_value_column).to_string(), | ||
| sql_with_multiple_value_column | ||
| ); |
There was a problem hiding this comment.
hmm this assertion is probably not needed, I think verified_stmt already does the same assertion
src/parser/mod.rs
Outdated
| let value_column = if self.peek_token_ref().token == Token::LParen { | ||
| self.parse_parenthesized_compound_identifier_list(Mandatory, false)? | ||
| } else { | ||
| vec![Expr::CompoundIdentifier( | ||
| self.parse_period_separated(|p| p.parse_identifier())?, | ||
| )] | ||
| }; |
There was a problem hiding this comment.
similar to unpivot can we call parse_expr directly here?
There was a problem hiding this comment.
no, we cannot do it here. (name, age) IN (('John', 30) AS c1, ('Mike', 40) AS c2) will be parsed as an expr.
There was a problem hiding this comment.
Ah right that makes sense, I think we can do something this instead to reuse existing capabilities of the parser?
let value_column = if self.peek_token_ref().token == Token::LParen {
self.parse_parenthesized_column_list_inner(Mandatory, false, |p| {
p.parse_subexpr(self.dialect.prec_value(Precedence::Between))
})?
} else {
vec![self.parse_subexpr(self.dialect.prec_value(Precedence::Between))?]
};
src/parser/mod.rs
Outdated
| let value_column = if self.peek_token_ref().token == Token::LParen { | ||
| self.parse_parenthesized_compound_identifier_list(Mandatory, false)? | ||
| } else { | ||
| vec![Expr::CompoundIdentifier( | ||
| self.parse_period_separated(|p| p.parse_identifier())?, | ||
| )] | ||
| }; |
There was a problem hiding this comment.
Ah right that makes sense, I think we can do something this instead to reuse existing capabilities of the parser?
let value_column = if self.peek_token_ref().token == Token::LParen {
self.parse_parenthesized_column_list_inner(Mandatory, false, |p| {
p.parse_subexpr(self.dialect.prec_value(Precedence::Between))
})?
} else {
vec![self.parse_subexpr(self.dialect.prec_value(Precedence::Between))?]
};
tests/sqlparser_common.rs
Outdated
| @@ -11143,7 +11146,7 @@ fn parse_pivot_unpivot_table() { | |||
| expr: call("sum", [Expr::Identifier(Ident::new("population"))]), | |||
| alias: None | |||
| }], | |||
| value_column: vec![Ident::new("year")], | |||
| value_column: vec![Expr::CompoundIdentifier(vec![Ident::new("year")])], | |||
There was a problem hiding this comment.
hmm heads up that this representation wouldn't be ideal - a compound identifier would be expected to have more than one ident in it
| @@ -10875,7 +10875,10 @@ fn parse_pivot_table() { | |||
| expected_function("b", Some("t")), | |||
There was a problem hiding this comment.
can we add tests to demonstrate the new behavior? it seems like those are currently lacking in the PR?
https://spark.apache.org/docs/latest/sql-ref-syntax-qry-select-pivot.html
currently this sql is not supported.